Add configuration option to turn off skeleton pack generation by elenatanasoiu · Pull Request #2359 · github/vscode-codeql

elenatanasoiu · 2023-04-19T16:54:44Z

We'd like to make auto generation of QL packs the default behaviour in the extension.

We have two flows at the moment that only work in the codespaces-codeql template:

the skeleton wizard flow which triggers when you run the CodeQL: Create query command
the database panel flow which offers to create a QL pack for you when you download a database from GitHub (see prompt below)

For the second flow, we should offer the user a third option called "No, and never ask me again".

This will disable auto generation. We are writing this choice to a VS Code setting called codeQL.createQuery.autogenerateQlPacks and checking for it before deciding whether to prompt the user again.

Screen.Recording.2023-04-19.at.18.51.47.mov

shati-patel

Looks good overall, thanks! 🍫 🥐

I've just left some questions inline about the "format" of the config option ⚙

robertbrignull · 2023-04-20T14:59:42Z

I really like that we're making this configurable. I'm sorry for being nit-picky about the definition of it, but since this is going to become user configuration, we want to be extra sure about it because we want to avoid changing it in the future.

elenatanasoiu · 2023-04-20T15:19:06Z

I'm sorry for being nit-picky about the definition of it

No need to apologise @robertbrignull! I enjoy getting feedback from you 👍

In the next commit we will write to this setting when the user chooses to disable the QL pack generation and prefers to never be asked again about it.

By writing the setting to our new `codeQL.autogenerateQlPacks` setting.

We modify both these options in the beforeEach(). We should reset them in the afterEach().

…ion`

To make it clear what it's supposed to be storing.

…teQlPacks`

I initially started defining an enum, but I'm not able to import it in package.json, where the list of options is defined (under `codeQL.createQuery.autogenerateQlPacks`).

elenatanasoiu · 2023-04-27T12:49:38Z

Thanks for the great explanations @shati-patel & @robertbrignull! 🙇‍♀️

I've had to rebase to account for file changes in the extension.

Bar any CI woes, this is ready for a re-review.

shati-patel

Thanks for the updates! ✨ A few more places to change, using the new enum 😎

elenatanasoiu · 2023-04-27T15:43:07Z

Thanks @shati-patel ❤️ I think it should be correct now.

shati-patel · 2023-04-27T15:51:13Z

Thanks @shati-patel ❤️ I think it should be correct now.

Nice! Looks good to me.

@robertbrignull, can I punt over to you for a final review/approval? (Since you suggested the new structure 🎈)

robertbrignull

I've had another look through and on the second look I've spotted a few things that could use some more attention. I think this PR is definitely heading in the right direction, but we risk making more bugs for ourselves and a confusing interface for users if we try to go too fast.

Since it's not defining a real option, we can get rid of it.

And update descriptions for them.

robertbrignull

One comment that needs updating after we changed the definition of things, but otherwise LGTM from just looking at the code.

I have tried to test this and I can't get the code to trigger, but I think it's because I'm not used to working with the codespace. Have you tested this, especially since we made a lot of modifications in review?

elenatanasoiu · 2023-05-03T15:10:30Z

Have you tested this, especially since we made a lot of modifications in review?

Yes, you have to uncomment "${workspaceRoot}/../codespaces-codeql/tutorial.code-workspace" from launch.json and make sure you've run npm run build.

I've just tested this and the setting was written correctly. I also tested that we stop asking if the setting is set to "never". I can follow up with a recording.

Thanks for the thorough review!

elenatanasoiu · 2023-05-03T16:04:55Z

Here's a recording of the functionality:
https://github.com/github/vscode-codeql/assets/1354439/6ad19134-945a-4c80-9a17-e7155ead9762

elenatanasoiu requested a review from a team as a code owner April 19, 2023 16:54

elenatanasoiu added the secexp label Apr 19, 2023

elenatanasoiu requested a review from a team April 19, 2023 16:57

elenatanasoiu force-pushed the yer-a-ternary-choice-query branch 2 times, most recently from 9e6fe9c to e99f5ef Compare April 19, 2023 17:07

elenatanasoiu changed the title ~~Add CodeQL extension configuration option to turn off skeleton pack generation~~ Add configuration option to turn off skeleton pack generation Apr 19, 2023

shati-patel reviewed Apr 20, 2023

View reviewed changes

Comment thread extensions/ql-vscode/package.json Outdated

Comment thread extensions/ql-vscode/package.json Outdated

Comment thread extensions/ql-vscode/test/vscode-tests/no-workspace/helpers.test.ts Outdated

Comment thread extensions/ql-vscode/test/vscode-tests/no-workspace/helpers.test.ts

robertbrignull reviewed Apr 20, 2023

View reviewed changes

Comment thread extensions/ql-vscode/package.json Outdated

robertbrignull reviewed Apr 20, 2023

View reviewed changes

Comment thread extensions/ql-vscode/src/config.ts Outdated

elenatanasoiu added 11 commits April 27, 2023 12:33

Give modal methods their own describe blocks

3eef0e9

Set up new config setting for auto generating QL packs

ae320d0

In the next commit we will write to this setting when the user chooses to disable the QL pack generation and prefers to never be asked again about it.

Introduce new dialog box with "No, never ask me again" option

8392284

Remember user's choice to not be prompted again

f260143

By writing the setting to our new `codeQL.autogenerateQlPacks` setting.

Fix error in skeleton wizard tests

c877f6c

We modify both these options in the beforeEach(). We should reset them in the afterEach().

Rename codeQL.createQuery.folder -> `codeQL.createQuery.qlPackLocat…

4e2b26f

…ion`

Rename SKELETON_WIZARD_FOLDER -> QL_PACK_LOCATION

9aeb520

To make it clear what it's supposed to be storing.

Rename codeQL.autogenerateQlPacks -> `codeQL.createQuery.autogenera…

6034105

…teQlPacks`

Convert autogenerateQlPacks to use enum

025ec25

Correct test descriptions to say ternary instead of binary

c71a83b

Fix comment to list correct options

d242117

I initially started defining an enum, but I'm not able to import it in package.json, where the list of options is defined (under `codeQL.createQuery.autogenerateQlPacks`).

elenatanasoiu force-pushed the yer-a-ternary-choice-query branch from 0d684e8 to d242117 Compare April 27, 2023 12:35

shati-patel reviewed Apr 27, 2023

View reviewed changes

Comment thread extensions/ql-vscode/src/databases/local-databases.ts Outdated

Comment thread extensions/ql-vscode/src/databases/local-databases.ts Outdated

Comment thread extensions/ql-vscode/test/vscode-tests/minimal-workspace/local-databases.test.ts Outdated

Write correct value to setting

f10185a

elenatanasoiu force-pushed the yer-a-ternary-choice-query branch from c2a0b62 to f10185a Compare April 27, 2023 15:42

robertbrignull reviewed Apr 27, 2023

View reviewed changes

elenatanasoiu added 5 commits May 3, 2023 07:35

Remove "default" option

9c47a27

Since it's not defining a real option, we can get rid of it.

Remove extra space

62597f1

Use parent setting for "Create Query" config

c8b0b1c

Use strictly type choices for config setting

530ae68

Don't try to restore setting

42320fc

elenatanasoiu force-pushed the yer-a-ternary-choice-query branch 3 times, most recently from 53d313e to 8220541 Compare May 3, 2023 09:12

Reduce options to "ask" and "never"

9ecb503

And update descriptions for them.

elenatanasoiu force-pushed the yer-a-ternary-choice-query branch from 8220541 to 9ecb503 Compare May 3, 2023 12:14

robertbrignull approved these changes May 3, 2023

View reviewed changes

Comment thread extensions/ql-vscode/src/config.ts Outdated

Update comment

a85d9d1

elenatanasoiu force-pushed the yer-a-ternary-choice-query branch from ffd3e76 to a85d9d1 Compare May 3, 2023 15:07

Merge branch 'main' into yer-a-ternary-choice-query

cc8f1fd

Merge branch 'main' into yer-a-ternary-choice-query

9515e3b

elenatanasoiu enabled auto-merge May 4, 2023 07:34

elenatanasoiu merged commit 3a5a81a into main May 4, 2023

elenatanasoiu deleted the yer-a-ternary-choice-query branch May 4, 2023 07:53

Conversation

elenatanasoiu commented Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shati-patel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robertbrignull commented Apr 20, 2023

Uh oh!

elenatanasoiu commented Apr 20, 2023

Uh oh!

elenatanasoiu commented Apr 27, 2023

Uh oh!

shati-patel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elenatanasoiu commented Apr 27, 2023

Uh oh!

shati-patel commented Apr 27, 2023

Uh oh!

robertbrignull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robertbrignull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elenatanasoiu commented May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elenatanasoiu commented May 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

elenatanasoiu commented Apr 19, 2023 •

edited

Loading

elenatanasoiu commented May 3, 2023 •

edited

Loading